Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix for minimal-printf when printing sign of double between 0.0 and -1.0 #13872

Closed
wants to merge 1 commit into from
Closed

Fix for minimal-printf when printing sign of double between 0.0 and -1.0 #13872

wants to merge 1 commit into from

Conversation

helpfulchicken
Copy link

@helpfulchicken helpfulchicken commented Nov 5, 2020 •
edited
Loading

Summary of changes

Fixes "minimal-printf doesn't display decimals between -1 to 0 correctly" listed in #13783

This is pull request is a suggested alternative to #13865, which also succeeds in fixing the above issue.

In mbed_minimal_formatted_string_double(), a given value of type double is printed in two parts, first the integer part and then the decimal part. The integer part is printed using mbed_minimal_formatted_string_signed(), by first casting the double datatype value into an integer datatype. For any values between 0.0 and -1.0, this casting produces the integer 0. Print the integer 0 does not result in a '-' sign, so the overall printing function for the double datatype prints values between 0.0 and -1.0 as positive values. E.g. printing the double value "-0.0567" produces a positive value "0.0567".

Impact of changes

Migration actions required

Documentation

None

Pull request type 

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR


double signed_small_decimal = -0.0567;
printf("value = %f\r\n", signed_small_decimal);

// Should print "value = -0.0567" rather than "value = 0.0567".
// I might have the default number of decimal places shown wrong.
// I think that this might also require enabling floating point values for minimal-printf.

--->

value = -0.0567

Reviewers

@0xc0170

@helpfulchicken
Copy link
Author

I thought it would be worth submitting my pull request for consideration. It's only my second pull request ever, so I'm not sure if I've done it right, feel free to edit it.

@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Nov 5, 2020
@ciarmcom ciarmcom requested review from 0xc0170 and a team November 5, 2020 23:00
@ciarmcom
Copy link
Member

ciarmcom commented Nov 5, 2020

@helpfulchicken, thank you for your changes.
@0xc0170 @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@helpfulchicken helpfulchicken force-pushed the FixMinimalPrintfSignedDouble branch from d2bfc11 to 105c39e Compare November 6, 2020 01:21
@helpfulchicken
Copy link
Author

I saw that it failed a style check, so I've fixed that and squashed the commits back down to a single commit.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 6, 2020

Thank you! We will review

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 6, 2020

@Linguanghsou Please review this alternative

@mergify mergify bot added needs: CI and removed needs: review labels Nov 11, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 11, 2020

@helpfulchicken can you add a test case to catch this bug: https://github.com/ARMmbed/mbed-os/tree/105c39e693e48f6bfe6a0ef8f1a608f4652b59d5/platform/tests/TESTS/mbed_platform/minimal-printf/compliance ? There are tests for minimal printf. We must have missed this use case if the test passed previously.

@Linguanghsou
Copy link

@Linguanghsou Please review this alternative

@0xc0170 I think helpfulchicken's fix is also good.

@helpfulchicken
Copy link
Author

@helpfulchicken can you add a test case to catch this bug: https://github.com/ARMmbed/mbed-os/tree/105c39e693e48f6bfe6a0ef8f1a608f4652b59d5/platform/tests/TESTS/mbed_platform/minimal-printf/compliance ? There are tests for minimal printf. We must have missed this use case if the test passed previously.

I will do this. It's been a busy time for me lately, but I haven't forgotten about this.

@evedon evedon mentioned this pull request Nov 28, 2020
Merged
4 tasks
@evedon
Copy link
Contributor

evedon commented Nov 28, 2020

Hi @helpfulchicken thank you for your fix. I reviewed #13548 and your fix will need to be adapted in that PR which adds support for width modifier, see my comment #13548 (comment)
So I don't think it makes sense to add the missing test case here.

Thank you very much for finding the issue and providing a solution; this is much appreciated.

@evedon
Copy link
Contributor

evedon commented Dec 7, 2020

This PR should be closed in favour of #13548

@0xc0170 0xc0170 closed this Dec 9, 2020
@mergify mergify bot added needs: CI and removed needs: preceding PR needs: work release-type: patch Indentifies a PR as containing just a patch labels Dec 9, 2020
@adbridge adbridge removed the needs: CI label Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@helpfulchicken @ciarmcom @0xc0170 @Linguanghsou @evedon @adbridge